Fixed conformance reporting #3348
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3348 +/- ##
==========================================
- Coverage 47.24% 45.42% -1.82%
==========================================
Files 476 483 +7
Lines 46892 49768 +2876
==========================================
+ Hits 22154 22609 +455
- Misses 24738 27159 +2421 ☔ View full report in Codecov by Sentry. |
boa_tester/src/main.rs
Outdated
| strict: Option<TestOutcomeResult>, | ||
| #[serde(rename = "r", default)] | ||
| no_strict: Option<TestOutcomeResult>, |
There was a problem hiding this comment.
We could simplify this by making TestOutcomeResult something like:
#[serde(tag = "t", content = "d")
enum TestOutcomeResult {
#[serde(rename = "O")]
Passed,
#[serde(rename = "I")]
Ignored,
#[serde(rename = "F")]
Failed {
strict: bool,
reason: String
},
#[serde(rename = "P")]
Panic {
strict: bool,
reason: String,
}
}Then, we won't need separate fields for strict and no_strict, just a single result.
There was a problem hiding this comment.
I was thinking about that, and also about the file size & JS performance in the browser. I think that having dictionaries with test/suite names as keys would decrease the size of the JSON around a 15%.
Booleans occupy a bit more. Maybe adding more variants, such as "StrictFailed" and so on could be more performant.
There was a problem hiding this comment.
Maybe it's time to transition to a binary serialization format such as MessagePack?
Another idea I had was to use Postcard and a small wasm glue code to deserialize the postcard and serialize it to JSON, then we would call that function from the conformance page.
There was a problem hiding this comment.
I'll play a bit with those
There was a problem hiding this comment.
I've given this a thought, and it could be a nice enhancement in the future, but I would for now just use JSON until the new website is up and running. What do you think?
There was a problem hiding this comment.
Yep, we can revisit this when we finish the site redesign.
Putting that aside, I still think it would be better to have FailedStrict and PanicStrict for simplicity, if we do use that info. If we don't, maybe we can obviate the need for those variants and just have a single result field?
|
I converted it back to draft, because I'm still missing:
|
Razican
force-pushed
the
real_conformance
branch
from
391416e to
b68e588
Compare
|
Since we will probably do a release before this is finished, and we need to fix our tester before then, I'll open a PR with a simpler version of this that just fixes the bug without changing the data representation too much. We can apply the changes of this PR after that. |
Razican
force-pushed
the
real_conformance
branch
from
0ba1929 to
3a8360b
Compare
|
This has been rebased. It's still missing:
|
We received reports in our Discord saying that our conformance representation was wrong.
We count a test that needs to be run both in strict and non-strict mode as 2 tests, and if it passes in both modes, we count 2 tests as passing, while if only one of them passes, we count one as passed, one as failed.
The correct interpretation is that a test is only considered passing if both strict and no-strict modes pass (tc39/test262#3871).
It changes the following:
TestResultnow contains 2 fields:strictandno_strict.strictis serialized ass, andno_strictis serialized asr(from "result")TestResultstructure, since it was being skipped and never usedThis requires a change in the conformance page to adhere to the new result format, and we need to migrate the current conformance results. I'll open a new PR for that.
This PR will fail when doing the result comparison, thoguh, so we might need to first merge the new result format, then this.